Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[android] fix the android build #757

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

hyp
Copy link
Contributor

@hyp hyp commented Jul 23, 2024

Macros can't yet be built as there's no host build support yet

@hyp
Copy link
Contributor Author

hyp commented Jul 23, 2024

@swift-ci please test

@hyp
Copy link
Contributor Author

hyp commented Jul 23, 2024

@swift-ci please test

guard let stream = fts_open(dirList.baseAddress!, opts, nil) else {
state = [Optional(UnsafeMutablePointer(mutating: path)), nil].withUnsafeBufferPointer { dirList in
#if canImport(Android)
let baseAddress = unsafeBitCast(dirList.baseAddress!, to: UnsafePointer<UnsafeMutablePointer<CChar>>.self)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate a bit more on what error this change resolves? I would have assumed that the above optional wrapping wouldn't be necessary since it'd be inferred by the compiler, and it seems that this unsafeBitCast of an optional pointer to a non-optional pointer would be a trap waiting for a caller that tries to read that final nil element, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed because the Android NDK has a nullability annotation on fts_open:

FTS* _Nullable fts_open(char* _Nonnull const* _Nonnull __path, int __options, int (* _Nullable __comparator)(const FTSENT* _Nonnull * _Nonnull  __lhs, const FTSENT* _Nonnull * _Nonnull __rhs)) __INTRODUCED_IN(21);

This makes the path pointer nonnullable . Therefore the type inference is then unable to infer the type for the array, and thus I need to explicitly make the first element optional, so that the second one can be null. Then the pointer need to be cast to match the type, as we still want to have a nil element in the array, but have non null pointer types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general this stems from the fact that this specific nullability annotation in the NDK is probably incorrect, as fts_open is documented to take an array that has NULL as the last element.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment in source.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should no longer be needed after swiftlang/swift#74829, as noted on your earlier commit weeks ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I need to retry with that change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the additional linux change that I mentioned there, we'll need to do something like that for that change to register on the linux CI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, that pull isn't needed at all on the linux CI, as this function signature only varies in Bionic.

In that case, this pull can just be closed, if #714 gets macros working and another small apinotes pull avoids the need for this change.

@hyp
Copy link
Contributor Author

hyp commented Jul 23, 2024

@swift-ci please test

@compnerd
Copy link
Member

I think that we should consider #714 as an alternative here as that is a broader fix.

@hyp
Copy link
Contributor Author

hyp commented Jul 26, 2024

This now drops the fts_open change - as posix_filesystem.apinotes are now picked up correctly : swiftlang/swift#75494

@hyp
Copy link
Contributor Author

hyp commented Jul 26, 2024

I think that we should consider #714 as an alternative here as that is a broader fix.
@compnerd Good point, let me test that change for Android instead.

Macros can't yet be built as there's no host build support yet
@hyp
Copy link
Contributor Author

hyp commented Jul 26, 2024

@swift-ci please test

1 similar comment
@hyp
Copy link
Contributor Author

hyp commented Jul 26, 2024

@swift-ci please test

@hyp
Copy link
Contributor Author

hyp commented Jul 29, 2024

Ok, macros work for android with:

compnerd#1
swiftlang/swift#75549

If we land host macros first, we can drop this PR. wdyt @compnerd, or should this land first .

@hyp
Copy link
Contributor Author

hyp commented Aug 5, 2024

I talked to @compnerd and I think we can hold off from this one, as the host macros changes should land soon.

@compnerd
Copy link
Member

@hyp I think that this can be closed now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants